Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow overriding of DWN hosts #93

Merged
merged 7 commits into from
May 28, 2023
Merged

allow overriding of DWN hosts #93

merged 7 commits into from
May 28, 2023

Conversation

michaelneale
Copy link
Contributor

@michaelneale michaelneale commented May 25, 2023

adds this enhancement: #86

Note: @TBD54566975/tbd-developer-relations external API changes:

  • Web5.connect() now takes an options object which is documented here

@csuwildcat
Copy link
Member

Would rather call the property dwnEndpoints, as host makes it seem like an external entity is always involved, when it could simply be a static IP to a box in your own home.

@michaelneale
Copy link
Contributor Author

@csuwildcat yeah I agree - was struggling for a name. endpoints is also what DID spec calls them, will change.

@michaelneale
Copy link
Contributor Author

@csuwildcat how does this look?

@csuwildcat
Copy link
Member

Lgtm

@frankhinek
Copy link
Contributor

@mistermoe were you going to push up some additions to the PR we discussed yesterday to make overriding other properties possible? Or should we start with dwnEndpoints and open a separate PR?

@mistermoe
Copy link
Contributor

i didn't get a chance to add those additional overrides we discussed yesterday. let's start with dwnEndpoints. i'll open a separate PR that introduce the other options

@mistermoe
Copy link
Contributor

the only thing that gives me pause at the moment is that this dwnEndpoints override option only makes sense in a scenario where an embedded user agent is being used. That said, the only agent that exists today is an embedded agent haha! the long term goal is for desktop & mobile agents to exist and it's in those external agents where you'd be providing your custom dwn endpoints or any other custom did configurations. that wouldn't happen via the web5 api.

Given that we haven't yet finished the first external agent, i think there's value in allowing people to provide custom dwn endpoints today. I think we could do it like so:

const { web5, did } = await Web5.connect({
  techPreview: {
    dwnEndpoints: []
 }
});

namespacing the dwnEndpoints option within a techPreview object hopefully makes it more clear that the option is temporary

README.md Outdated Show resolved Hide resolved
@frankhinek
Copy link
Contributor

Spotted a couple issues with the existing PR:

  • only 1 of the 2 README files was updated
  • README example was const { web5, did: myDid } = await connect(... instead of const { web5, did: myDid } = await Web5.connect(....

both resolved with Commit 05e4e34

Still one remaining issue which is if dwnEndpoints is an empty array, the messageAuthorizationKeys and recordEncryptionKeys values aren't populated in the DID document:

with { techPreview: { dwnEndpoints: [] } }:

Screenshot 2023-05-26 at 12 52 46 PM

with { techPreview: { dwnEndpoints: ['https://dwn.your-domain.org'] } }:

Screenshot 2023-05-26 at 12 55 53 PM

Discussing with @mistermoe and @csuwildcat as to how we should handle.

@frankhinek frankhinek force-pushed the default_dwn_hosts branch from c6ddd50 to b259d24 Compare May 28, 2023 00:41
packages/web5/src/web5.ts Outdated Show resolved Hide resolved
packages/web5/src/web5.ts Outdated Show resolved Hide resolved
@michaelneale
Copy link
Contributor Author

yeah calling connect with dwnEndpoints specified is in some ways assuming that is the special case that a did will be created for you, but wasn't sure where else it belongs in the short term.

Co-authored-by: Frank Hinek <[email protected]>
Co-authored-by: Moe Jangda <[email protected]>
@frankhinek frankhinek added bug Something isn't working enhancement New feature or request labels May 28, 2023
@frankhinek frankhinek linked an issue May 28, 2023 that may be closed by this pull request
Copy link
Contributor

@mistermoe mistermoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇

@frankhinek frankhinek merged commit c1dce93 into main May 28, 2023
@frankhinek frankhinek deleted the default_dwn_hosts branch June 2, 2023 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working devrel-content-needed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make hosted DWNs and sync configurable
5 participants